fix: memoize channel(name) to make Channel identity stable per name#27
Merged
Conversation
The phony AVOID_GARBAGE_COLLECTION subscriber installed by this patch keeps a created Channel alive, but Node's underlying channel(name) can still return a brand-new Channel object for the same name on a subsequent call (observed on Node 18 under certain workloads — e.g. heavy proxyquire reload + plugin teardown/recreate cycles). Because the existing anti-GC tracking is a WeakSet keyed by Channel-object identity, each new object is treated as new and gets its own phony subscriber. The two objects are never unified. The visible failure mode for callers: code that captures a Channel in a module-level closure (a common pattern for instrumentation publishers) ends up publishing to a different Channel than later subscribers attach to. Publishes go nowhere; subscribers see nothing. hasSubscribers reports false (only the phony is on the publish-side Channel) and traceSync/traceCallback fast-path-bypass the publish. This fix memoizes dc.channel(name) by name and short-circuits before delegating to channel() on subsequent calls. Channel identity is now stable per name regardless of what the underlying channel() returns — the contract callers actually depend on. Empirically verified end-to-end against dd-trace-js's ws plugin stress workflow (10 runners × 25 iterations of plugin tests on Node 18.20.8, 250 runs total): 0/10 runners pass without this patch, 10/10 pass with it. dd-trace-js PR for context: DataDog/dd-trace-js#8297. All existing dc-polyfill tests pass (26/26 suites).
Adds a unit test that injects a mock unpatched module whose channel(name) returns a different Channel object on every call, simulating the Node 18 bug. Without the memoization in patch-garbage-collection-bug.js, this test fails 6/8 assertions (channel identity diverges, subscribers attach to a different Channel than later publishers, the underlying channel() is re-invoked on every memoizable lookup). With the patch, all 8 pass. This pins the contract the patch is meant to enforce — dc.channel(name) returns a stable Channel object per name regardless of what the underlying registry hands back — without requiring a real Node 18 bug to be triggered to exercise the patch.
bm1549
commented
May 8, 2026
6f0a888 to
deac894
Compare
rochdev
requested changes
May 8, 2026
Member
rochdev
left a comment
There was a problem hiding this comment.
The global subscribe was introduced to fix this. Let's use that and avoid patching a deprecated API. For versions that don't have it we should still do since it should also be in the polyfill. It's also possible to work around the issue in the integration itself by keeping a reference to channels instead of using channel() again every time (which is how this API was originally designed to be used in the first place).
rochdev
approved these changes
May 8, 2026
| module.exports = function(unpatched) { | ||
| const dc_channel = unpatched.channel; | ||
| const channels = new WeakSet(); | ||
| const byName = new Map(); |
Member
There was a problem hiding this comment.
This will grow forever but I think that's acceptable as channels should not be high cardinality and are pretty lightweight.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Memoizes
dc.channel(name)by name in patch-garbage-collection-bug.js, so subsequent calls for the same name always return the sameChannelobject regardless of what the underlying `channel()` returns. Also adds a regression test that pins the contract.module.exports = function(unpatched) { const dc_channel = unpatched.channel; const channels = new WeakSet(); + const byName = new Map(); const dc = { ...unpatched }; dc.channel = function() { + const name = arguments[0]; + if (byName.has(name)) return byName.get(name); const ch = dc_channel.apply(this, arguments); + byName.set(name, ch); if (channels.has(ch)) return ch;Motivation
The
PHONY_SUBSCRIBEanti-GC patch keeps a createdChannelalive, but Node's underlyingchannel(name)can still return a brand-newChannelobject for the same name on a subsequent call (observed on Node 18.20.8 under heavy workloads — e.g. proxyquire-driven reload of a tracer + plugin manager teardown/recreate cycles). The existing tracking — `channels = new WeakSet()` keyed by Channel-object identity — can't unify those: each new object isn't in the WeakSet, so dc-polyfill treats it as new, attaches its own phony, and returns it.The visible failure mode for callers (a common instrumentation pattern):
A separate consumer doing `dc.channel('foo').subscribe(...)` later may get a different
Channelinstance, attach its handler there, and never see publishes from the wrap — because the wrap's closure holds the earlierChannel. With only the phony left on the publish-sideChannel,hasSubscribersreturns false and the fast-path bypass kicks in. Publishes go nowhere.Why this fix lives here, not upstream in Node
The underlying bug is in Node's diagnostics_channel registry — the right architectural home for the fix is Node itself. It is not realistic to land the fix there, however:
hasFullSupport()check treats Node 20.6+ as fully supported (so this whole patch file is inert there). Empirically I see the same: dd-trace-js's reproducer triggers on Node 18.20.8 and not on Node 23.3.0 against the same code.Verification
Additional Notes
tracingChannelwrappers across dc-polyfill versions); this patch makes the underlying per-versionchannel()results stable.🤖 Generated with Claude Code